Sorting on models page by Last Modified is reflecting changes in versions as well #3597

YuliaKrimerman · 2024-12-19T17:18:28Z

Description

When you go to the Models list page, the sorting by Last modified will now reflect any changes in the versions of that model. To test this got to some model that is not on top of the list and edit some version on it on any possible field( labels, description etc). Return to the models page and notice that the model is now on top of the list. In addition test similar scenario but we creating a new model Version.

Screen.Recording.2024-12-19.at.9.53.52.AM.mov

How Has This Been Tested?

This was tested on MR cluster

Test Impact

Updated and added tests to cover the new hooks und functionalities (WIP)

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

mturley · 2024-12-19T20:10:05Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTableRow.tsx

@@ -127,15 +135,16 @@ const ModelVersionsTableRow: React.FC<ModelVersionsTableRowProps> = ({
          ) : null}
          {isDeployModalOpen ? (
            <DeployRegisteredModelModal
-              onSubmit={() =>
+              onSubmit={async () => {
+                await handleAfterDeploy();


I think you also need to add this to where DeployRegisteredModelModal gets rendered in ModelVersionDetailsHeaderActions

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx

mturley

A handful of things here that shouldn't be too bad to address - but this is already working nicely!

mturley · 2024-12-19T21:21:03Z

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx

+      return;
+    }
+
+    await bumpBothTimestamps(apiState.api, mv.id, mv.registeredModelId);


Since the version was just updated, you only need to bump the model timestamp, not both here.

I tried- It didn't work - need both of them .

I don't understand though, the updatePromise you pass in is already calling patchModelVersion with a description/labels, why do we need to patch it again to get the timestamp to change? Are you sure?

mturley · 2024-12-19T21:29:26Z

frontend/src/concepts/modelRegistry/utils/updateTimestamps.ts

+        customProperties: {
+          _lastModified: {
+            metadataType: ModelRegistryMetadataType.STRING,
+            // eslint-disable-next-line camelcase
+            string_value: currentTime,
+          },
+        },


What's up with this customProperties piece? I don't think it should be necessary, and it'll show up in the properties table in the UI

I think all we need is the { state } part.

Also... and this part is more annoying... I wonder if it's ok to just pass ModelState.LIVE here, technically if you called this on an archived model it would un-archive it. but since the UI also prevents editing archived stuff I think we're ok. We may just want a comment explaining that here.

Thinking more - if the _lastModified addition here was because the { state } alone wasn't enough to automatically bump the version, that's an issue we should raise again with the backend team. If we get blocked on this and it looks like it won't make 2.17 we could remove it from the strat scope. Or if we want a temporary workaround we can leave this, but we should maybe filter it out of the properties list so it doesn't look confusing, and open a followup issue we link in comments here and where that filter is added.

Let's add a comment here explaining the workaround and linking to https://issues.redhat.com/browse/RHOAIENG-17614

mturley · 2024-12-19T21:37:23Z

frontend/src/api/modelRegistry/custom.ts

+        patchRegisteredModel: (apiOpts, patchData, id) =>
+          handleModelRegistryFailures(
+            proxyPATCH(
+              hostpath,
+              `/api/model_registry/${MODEL_REGISTRY_API_VERSION}/registered_models/${id}`,
+              patchData,
+              apiOpts,
+            ),
+          ),
+      },
+      registeredModelId,
+    );


You might be able to just use the patchRegisteredModel exported from this same file below (already in scope inside this function), it returns the same function you're writing here.

await bumpRegisteredModelTimestamp( { patchRegisteredModel: patchRegisteredModel(hostpath) }, registeredModelId, );

frontend/src/concepts/modelRegistry/apiHooks/useModelVersionCreation.ts

mturley · 2024-12-19T21:41:54Z

frontend/src/concepts/modelRegistry/context/ModelRegistryContext.tsx

@@ -75,5 +81,9 @@ export const useModelRegistryAPI = (): UseModelRegistryAPI => {
  return {
    refreshAllAPI,
    ...apiState,
+    patchModelVersion: (modelVersionId: string, updates: Partial<ModelVersion>) =>
+      apiState.api.patchModelVersion({}, updates, modelVersionId).then(() => undefined),


I'm not sure I understand the purpose of the changes in this file. Why add these patch functions to the root of the returned object when they're already returned under .api.* which you use internally here? Why then and then do nothing in the callback?

mturley · 2024-12-19T21:51:58Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTable.tsx

+        await Promise.all([
+          modelRegistryApi.patchModelVersion(modelVersionId, { state: ModelState.LIVE }),
+          modelRegistryApi.patchRegisteredModel(registeredModelId, { state: ModelState.LIVE }),
+        ]);


If you remove the changes you made to ModelRegistryContext, you can just rewrite these calls using the stuff already returned by useModelRegistryAPI. You just need an extra .api.* and to rearrange your args a bit:

await Promise.all([ modelRegistryApi.api.patchModelVersion({}, { state: ModelState.LIVE }, modelVersionId), modelRegistryApi.api.patchRegisteredModel( {}, { state: ModelState.LIVE }, registeredModelId, ), ]);

I wonder if the reason for these extra functions was because you didn't see them exposed on the root of the returned modelRegistryApi object here? I think this is why in other places we name that variable apiState and then call apiState.api.patch.... It's structured that way so we can attach extra stuff like refreshAllAPI to it but the core .api object already included here should contain all you need.

Also - as mentioned elsewhere, since you'll need this same behavior for both of the places where we can deploy (the <DeployRegisteredModelModal> is also rendered in the ModelVersionDetailsHeaderActions component) you may not want to pass down a handleAfterDeploy from all the way up here (and have to duplicate it). Maybe you can just write this logic inside DeployRegisteredModelModal itself? That component is only used for model registry (it's a wrapper around the stuff we reuse in model serving) so it should always be bumping these timestamps. You can call useModelRegistryAPI(); in there. Then you could remove some of the extra plumbing you added to make handleAfterDeploy work.

Lastly, you can probably use your new util bumpBothTimestamps instead of repeating the patches here.

mturley · 2024-12-19T22:00:06Z

frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx

@@ -52,11 +52,10 @@ const DeployRegisteredModelModal: React.FC<DeployRegisteredModelModalProps> = ({
  } = useRegisteredModelDeployInfo(modelVersion);

  const onClose = React.useCallback(
-    (submit: boolean) => {
+    async (submit: boolean) => {


I'm not sure I see why this was made async, I can't find anywhere it's awaited and it's not awaiting anything

mturley

One more thing to clean up, and something I realize is a problem that's probably outside the scope of this PR to solve.

mturley · 2024-12-20T16:47:38Z

frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTable.tsx

+  const modelRegistryApi = useModelRegistryAPI();
+
+  const handleAfterDeploy = useCallback(
+    async (modelVersionId: string) => {
+      if (!registeredModelId) {
+        return;
+      }
+
+      try {
+        await Promise.all([
+          modelRegistryApi.api.patchModelVersion({}, { state: ModelState.LIVE }, modelVersionId),
+          modelRegistryApi.api.patchRegisteredModel(
+            {},
+            { state: ModelState.LIVE },
+            registeredModelId,
+          ),
+        ]);
+      } catch (error) {
+        throw new Error('Failed to update timestamps after deployment');
+      }
+    },
+    [registeredModelId, modelRegistryApi],
+  );
+


I think now that you're doing this logic inside DeployRegisteredModelModal you can remove all this from ModelVersionsTable, and also remove all the onAfterDeploy / handleAfterDeploy related code elsewhere right?

frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx

mturley · 2024-12-20T18:15:04Z

frontend/src/pages/modelRegistry/screens/components/DeployRegisteredModelModal.tsx

+      await Promise.all([
+        modelRegistryApi.api.patchModelVersion({}, { state: ModelState.LIVE }, modelVersion.id),
+        modelRegistryApi.api.patchRegisteredModel(
+          {},
+          { state: ModelState.LIVE },
+          modelVersion.registeredModelId,
+        ),
+      ]);


You still have direct patch calls here where I think you need to be using bumpBothTimestamps instead to make sure the _lastModified property workaround gets applied

mturley

Code LGTM, we'll hold this for now until we have tests -- and I'll give it another review after the holidays to be safe because my brain is fried 🫠 but it looks to be in a good place.

codecov · 2025-01-03T18:01:58Z

Codecov Report

Attention: Patch coverage is 75.40984% with 15 lines in your changes missing coverage. Please review.

Project coverage is 85.27%. Comparing base (14b1c4d) to head (1d7aba9).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx	61.11%	7 Missing ⚠️
.../screens/components/DeployRegisteredModelModal.tsx	33.33%	6 Missing ⚠️
...ry/screens/ModelVersions/ModelVersionsTableRow.tsx	0.00%	1 Missing ⚠️
frontend/src/pages/modelRegistry/screens/utils.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3597      +/-   ##
==========================================
+ Coverage   85.16%   85.27%   +0.11%     
==========================================
  Files        1393     1396       +3     
  Lines       31939    32092     +153     
  Branches     8955     8994      +39     
==========================================
+ Hits        27200    27367     +167     
+ Misses       4739     4725      -14

Files with missing lines	Coverage Δ
frontend/src/api/modelRegistry/custom.ts	`100.00% <100.00%> (ø)`
...c/concepts/modelRegistry/utils/updateTimestamps.ts	`100.00% <100.00%> (ø)`
...istry/screens/ModelVersions/ModelVersionsTable.tsx	`100.00% <ø> (ø)`
...ry/screens/ModelVersions/ModelVersionsTableRow.tsx	`94.23% <0.00%> (-1.93%)`	⬇️
frontend/src/pages/modelRegistry/screens/utils.ts	`93.02% <50.00%> (-1.03%)`	⬇️
.../screens/components/DeployRegisteredModelModal.tsx	`80.76% <33.33%> (-7.87%)`	⬇️
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx	`75.92% <61.11%> (-10.75%)`	⬇️

... and 29 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14b1c4d...1d7aba9. Read the comment docs.

mturley

Great stuff, thanks @YuliaKrimerman !

openshift-ci · 2025-01-06T16:49:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [mturley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from DaoDaoNoCode and ppadti December 19, 2024 17:18

YuliaKrimerman added 5 commits December 19, 2024 15:02

working version

537f7c2

Remove not-needed

52a8b05

working version

4583fc8

fixed lint

7277c59

fixed side effect for label

72d36df

YuliaKrimerman force-pushed the RHOAIENG-16077 branch from 2dadc96 to 72d36df Compare December 19, 2024 20:05

mturley reviewed Dec 19, 2024

View reviewed changes

removed files

3df03c7

YuliaKrimerman commented Dec 19, 2024

View reviewed changes

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx Outdated Show resolved Hide resolved

fixed rebase

82c76d0

mturley reviewed Dec 19, 2024

View reviewed changes

frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx Show resolved Hide resolved

fixing

0105b6f

mturley requested changes Dec 19, 2024

View reviewed changes

openshift-ci bot assigned mturley Dec 19, 2024

YuliaKrimerman added 2 commits December 20, 2024 10:51

addressed comments

db0a896

filtering and fixes

c01978e

mturley requested changes Dec 20, 2024

View reviewed changes

cleanup

419ab6d

mturley reviewed Dec 20, 2024

View reviewed changes

more comments

bb185fd

mturley reviewed Dec 20, 2024

View reviewed changes

tests

1d7aba9

YuliaKrimerman added 2 commits January 6, 2025 11:10

added Jira issue in comment

637fa40

added comment

76773f5

mturley approved these changes Jan 6, 2025

View reviewed changes

openshift-ci bot added the lgtm label Jan 6, 2025

openshift-ci bot added the approved label Jan 6, 2025

openshift-merge-bot bot merged commit d1c56d9 into opendatahub-io:main Jan 6, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorting on models page by Last Modified is reflecting changes in versions as well #3597

Sorting on models page by Last Modified is reflecting changes in versions as well #3597

YuliaKrimerman commented Dec 19, 2024

mturley Dec 19, 2024

mturley left a comment

mturley Dec 19, 2024

YuliaKrimerman Dec 20, 2024

mturley Dec 20, 2024

mturley Dec 19, 2024

mturley Dec 19, 2024 •

edited

Loading

mturley Jan 6, 2025

mturley Dec 19, 2024

mturley Dec 19, 2024

mturley Dec 19, 2024

mturley Dec 19, 2024

mturley Dec 19, 2024

mturley left a comment

mturley Dec 20, 2024

mturley Dec 20, 2024

mturley left a comment

codecov bot commented Jan 3, 2025

mturley left a comment

openshift-ci bot commented Jan 6, 2025

Sorting on models page by Last Modified is reflecting changes in versions as well #3597

Sorting on models page by Last Modified is reflecting changes in versions as well #3597

Conversation

YuliaKrimerman commented Dec 19, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2025

Codecov Report

mturley left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 6, 2025

mturley Dec 19, 2024 •

edited

Loading